Fix crash and performance issues in _dirs command - #209

Please address the comments from this code review:

## Overall Comments
- Changing `ShortestPathRecipient::virt_receiveShortestPath` to take `const QVector<SPNode>&` conflicts with the comment and likely behavior in `ShortestPathEmitter` where `spnode` is modified; if mutation of `SPNode` instances is required, consider taking a non-const reference to the vector or redesigning that usage to avoid mutating through a const container.

## Individual Comments

### Comment 1
<location path="src/parser/abstractparser.cpp" line_range="395-400" />
<code_context>

 private:
-    void virt_receiveShortestPath(QVector<SPNode> spnodes, const int endpoint) final
+    void virt_receiveShortestPath(const QVector<SPNode> &spnodes, const int endpoint) final
     {
-        assert(0 < endpoint && endpoint < spnodes.size());
+        assert(0 <= endpoint && endpoint < spnodes.size());

         // Caution: spnode is modified here.
         const SPNode *spnode = &spnodes[endpoint];
</code_context>
<issue_to_address>
**issue (bug_risk):** Signature now takes a const QVector, but the implementation comment indicates the elements are modified.

If `*spnode` is still mutated, this change will either not compile or force const_casts, which is confusing for callers. Either keep the parameter non-const if mutation is intended, or refactor the implementation to work on a copy so the parameter can safely remain const.
</issue_to_address>

### Comment 2
<location path="src/mapdata/shortestpath.h" line_range="29-32" />
<code_context>

 private:
-    virtual void virt_receiveShortestPath(QVector<SPNode> spnodes, int endpoint) = 0;
+    virtual void virt_receiveShortestPath(const QVector<SPNode> &spnodes, int endpoint) = 0;

 public:
-    void receiveShortestPath(QVector<SPNode> spnodes, const int endpoint)
+    void receiveShortestPath(const QVector<SPNode> &spnodes, const int endpoint)
     {
         virt_receiveShortestPath(spnodes, endpoint);
</code_context>
<issue_to_address>
**question (bug_risk):** Changing from pass-by-value to const-reference tightens lifetime expectations for `spnodes`.

Previously, passing `QVector<SPNode>` by value let implementations own and freely retain or modify their copy. With `const QVector<SPNode>&`, the data is now non-owning and only valid for the duration of the call. Any implementation that stores references/pointers into `spnodes` or assumes it outlives the function may now be unsafe. Please verify existing implementers of this interface don’t rely on longer lifetimes.
</issue_to_address>

Please address the comments from this code review:

## Overall Comments
- Changing `ShortestPathRecipient::receiveShortestPath` and `virt_receiveShortestPath` to take `const QVector<SPNode> &` introduces a lifetime requirement on the caller; consider enforcing this more explicitly (e.g., by passing a value, a shared container, or a view/span type) or documenting that the vector must outlive the recipient callback to avoid future dangling references.
- In `MapData::shortestPathSearch`, now that `thisr` is intentionally a copy, you may want to declare it as `const RoomHandle thisr = ...;` to make its immutability explicit and prevent accidental modifications that could obscure the reason for copying.

Please address the comments from this code review:

## Overall Comments
- The use of `visited` as a `std::vector<uint8_t>(map.getRoomsCount() + 1)` keyed by `RoomId::asUint32()` is a bit opaque—consider adding a brief comment or helper to document the invariant between `RoomId` values and `getRoomsCount()` (and why `+1` is safe) to prevent future off‑by‑one or out‑of‑range assumptions.
- There is a lot of casting between `int` and `std::size_t` for shortest‑path node indices (e.g., in `sp_nodes`, `endpoint`, and `parent`), which makes the code harder to reason about; consider standardizing on an unsigned index type (or a dedicated alias) and using a sentinel/optional instead of `-1` so you can drop the repeated casts and signed/unsigned checks.

## Individual Comments

### Comment 1
<location path="src/mapdata/shortestpath.cpp" line_range="118-127" />
<code_context>
+    std::vector<uint8_t> visited(map.getRoomsCount() + 1, 0);
</code_context>
<issue_to_address>
**issue (bug_risk):** RoomId-to-index mapping for `visited` assumes a tight ID range and can break visited semantics for out-of-range IDs.

This relies on `room_id.asUint32() <= map.getRoomsCount()` (plus the `+1` margin). If `RoomId` values are sparse or can exceed `getRoomsCount()`, any `room_id.asUint32() >= visited.size()` will never be tracked as visited, so those rooms may be re-expanded indefinitely and Dijkstra’s finalization invariant breaks. Unless the tight ID range is guaranteed, consider either sizing `visited` from the maximum internal ID, reverting to a set-based representation, or explicitly handling out-of-range IDs (e.g., early-continue with a clear policy) instead of silently skipping them.
</issue_to_address>

Please address the comments from this code review:

## Overall Comments
- Consider using `std::size_t` (or the container’s `size_type`) for `SPNodeIdx` instead of `std::uint32_t`, or at least adding a safeguard, to avoid potential truncation/overflow if the number of path nodes grows beyond 2^32−1.
- In `MapData::shortestPathSearch`, you now duplicate path reconstruction logic that previously lived in the recipient; if future consumers need different path projections (e.g., including nodes, not just directions), it might be worth centralizing or exposing a helper to avoid reimplementing the reconstruction in multiple places.

## Individual Comments

### Comment 1
<location path="src/parser/abstractparser.cpp" line_range="395-398" />
<code_context>

 private:
-    void virt_receiveShortestPath(QVector<SPNode> spnodes, const int endpoint) final
+    void virt_receiveShortestPath(const Map &map, ShortestPathResult result) final
     {
-        assert(0 < endpoint && endpoint < spnodes.size());
+        const auto room = map.getRoomHandle(result.id);
+        const auto name = room ? room.getName() : RoomName{};

-        // Caution: spnode is modified here.
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Swallowing a missing room silently could make underlying data issues harder to detect.

Since virt_receiveShortestPath now falls back to an empty RoomName when map.getRoomHandle(result.id) fails, any missing room will appear as a "normal" case. Because the search code should only emit results for existing rooms, hitting this path likely indicates a data inconsistency. Consider asserting or logging a warning instead of silently using an empty name, so these issues are visible during development and debugging.

Suggested implementation:

```cpp
    void virt_receiveShortestPath(const Map &map, ShortestPathResult result) final
    {
        const auto room = map.getRoomHandle(result.id);
        if (!room) {
            qWarning() << "ShortestPathEmitter::virt_receiveShortestPath received result for non-existent room id"
                       << result.id;
            return;
        }
        const auto name = room.getName();

        parser.sendToUser(SendToUserSourceEnum::FromMMapper,
                          "Distance " + std::to_string(result.dist) + ": " + name.toStdStringUtf8()
                              + "\n");

```

If `qWarning` is not already used in this file, ensure the appropriate Qt debug header is included, e.g. `#include <QDebug>`, near the top of `src/parser/abstractparser.cpp`. This will make the missing-room cases visible during development and debugging while avoiding undefined behavior in release builds.
</issue_to_address>

### Comment 2
<location path="tests/TestShortestPath.cpp" line_range="124-129" />
<code_context>
+    QVERIFY(optFilter.has_value());
+    TestRecipient recipient;
+
+    MapData::shortestPathSearch(r1_handle, recipient, *optFilter, 1, 0);
+
+    QCOMPARE(recipient.results.size(), 1ULL);
+    QCOMPARE(recipient.results[0].path.size(), 2ULL);
+    QCOMPARE(recipient.results[0].path[0], ExitDirEnum::NORTH);
+    QCOMPARE(recipient.results[0].path[1], ExitDirEnum::EAST);
+
+    // Test including start room
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test covering `max_dist` cutoff behavior in `shortestPathSearch`

The updated code adds explicit `max_dist` handling, but this test always uses `max_dist = 0` (no limit), so that code path isn’t covered. Please add a case with a finite `max_dist` that permits the first step but not the second (or vice versa), and assert that farther rooms are excluded. This will protect against regressions in cost accumulation and the `max_dist` early-return behavior.
</issue_to_address>

Please address the comments from this code review:

## Overall Comments
- In `MapData::shortestPathSearch`, exits pointing to non-existent rooms are now silently skipped; consider keeping at least a `qWarning()` here to preserve diagnostics for corrupted or out-of-sync maps while still avoiding crashes.
- The fixed `INITIAL_NODES_CAPACITY` of 1024 for the shortest-path node vector may be suboptimal for very small or very large maps; you might want to derive this from map size or make it configurable to avoid over-allocation or repeated growth.

## Individual Comments

### Comment 1
<location path="tests/TestShortestPath.cpp" line_range="19-28" />
<code_context>
+#include <QDebug>
+
 std::ostream &operator<<(std::ostream &os, const RoomId id)
 {
     return os << "RoomId(" << id.value() << ")";
</code_context>
<issue_to_address>
**suggestion (testing):** Also assert on the computed distances to validate movement cost handling, not just directions

This block depends on the indoor terrain cost (0.75 per step) and the new cost constants, but the test only verifies the direction sequence. Please also assert the computed distance (e.g. `QCOMPARE(recipient.results[0].dist, 1.5);` or a fuzzy FP check) so regressions in `terrain_cost` or `getLength` are caught.
</issue_to_address>

Please address the comments from this code review:

## Overall Comments
- In `MapData::shortestPathSearch`, when `map.getRoomHandle(nextrId)` fails the code now silently `continue`s; consider restoring at least a `qWarning` (without asserting) so unexpected broken exits remain diagnosable at runtime instead of failing quietly.